-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Conversation
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
@@ -417,8 +416,8 @@ fn reduce_all<A: IdentifierT>(assignments: &mut Vec<StakedAssignment<A>>) -> u32 | |||
// find minimum of cycle. | |||
let mut min_value: ExtendedBalance = Bounded::max_value(); | |||
// The voter and the target pair that create the min edge. | |||
let mut min_target: A = Default::default(); | |||
let mut min_voter: A = Default::default(); | |||
let mut min_target: Option<A> = None; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About this: after looking at the code, this bit seems like the only controversial bit. My conclusion is that this diff is not changing the logic of this snippet at all. The Default::default()
, and min_index = 0
were clearly meant to be overwritten in the for loop
that follows. In fact, the outcome is (pretty bad) UB if it doesn't. The changes of this PR are not altering any of that. The assumption still must hold that these values get written to. Making them an Option<_>
doesn't harm that.
I wish I had the time now, or in the near future, to re-write the majority of this reduce
algorithm. Admittedly, it is the last historical Spaghetti code left in sp-npos-election
that I myself even have a very hard time reading. It was mostly written during a crunch and never been re-visited ever since. While I can't take the time now to improve this code fundamentally, I did propose some reasonable changes in this branch to make it more compatible with Option<_>
, most notably making the assumption explicit that these Option
will all become Some
and stop the algorithm otherwise.
Lastly, luckily this code has a fuzzer that has proven quite helpful in the past. I'll re-run it again on my aforementioned branch and ring some bells if anything is broken.
Feel free to either apply the commits of https://github.com/paritytech/substrate/compare/gav-no-default-accountid...gav-no-default-accountid-staking?expand=1 either directly here, or I can open a PR on top of this. All changes in it are pretty clear in my opinion.
* Remove Default for AccountId * More removals of default * Update frame/authorship/src/lib.rs Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> * Update frame/authorship/src/lib.rs Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> * Update frame/authorship/src/lib.rs Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> * Update frame/authorship/src/lib.rs Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> * More work * More work * Remove old code * More work * pallet-asset-tx-payment * tips * sc-consensus-babe * sc-finality-grandpa * sc-consensus-babe-rpc * sc-cli * make npos crates accept non-default account (paritytech#10420) * minimal changes to make npos pallets all work * make this pesky reduce.rs a bit cleaner * more work * more work * Tests build * Fix imonline tests * Formatting * Fixes * Fixes * Fix bench * Fixes * Fixes * Fixes * Fixes * Fixes * Formatting * Fixes * Formatting * Fixes * Formatting * Fixes * Formatting * Fixes * Formatting * Update client/keystore/src/local.rs Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> * Update client/finality-grandpa/src/lib.rs Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> * Update client/keystore/src/local.rs Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> * Update client/keystore/src/local.rs Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> * Update frame/staking/src/lib.rs Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> * Update frame/staking/src/lib.rs Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> * Update primitives/runtime/src/traits.rs Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> * Formatting Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Co-authored-by: kianenigma <kian@parity.io>
* Remove Default for AccountId * More removals of default * Update frame/authorship/src/lib.rs Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> * Update frame/authorship/src/lib.rs Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> * Update frame/authorship/src/lib.rs Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> * Update frame/authorship/src/lib.rs Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> * More work * More work * Remove old code * More work * pallet-asset-tx-payment * tips * sc-consensus-babe * sc-finality-grandpa * sc-consensus-babe-rpc * sc-cli * make npos crates accept non-default account (paritytech#10420) * minimal changes to make npos pallets all work * make this pesky reduce.rs a bit cleaner * more work * more work * Tests build * Fix imonline tests * Formatting * Fixes * Fixes * Fix bench * Fixes * Fixes * Fixes * Fixes * Fixes * Formatting * Fixes * Formatting * Fixes * Formatting * Fixes * Formatting * Fixes * Formatting * Update client/keystore/src/local.rs Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> * Update client/finality-grandpa/src/lib.rs Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> * Update client/keystore/src/local.rs Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> * Update client/keystore/src/local.rs Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> * Update frame/staking/src/lib.rs Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> * Update frame/staking/src/lib.rs Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> * Update primitives/runtime/src/traits.rs Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> * Formatting Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Co-authored-by: kianenigma <kian@parity.io>
* Remove Default for AccountId * More removals of default * Update frame/authorship/src/lib.rs Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> * Update frame/authorship/src/lib.rs Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> * Update frame/authorship/src/lib.rs Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> * Update frame/authorship/src/lib.rs Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> * More work * More work * Remove old code * More work * pallet-asset-tx-payment * tips * sc-consensus-babe * sc-finality-grandpa * sc-consensus-babe-rpc * sc-cli * make npos crates accept non-default account (paritytech#10420) * minimal changes to make npos pallets all work * make this pesky reduce.rs a bit cleaner * more work * more work * Tests build * Fix imonline tests * Formatting * Fixes * Fixes * Fix bench * Fixes * Fixes * Fixes * Fixes * Fixes * Formatting * Fixes * Formatting * Fixes * Formatting * Fixes * Formatting * Fixes * Formatting * Update client/keystore/src/local.rs Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> * Update client/finality-grandpa/src/lib.rs Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> * Update client/keystore/src/local.rs Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> * Update client/keystore/src/local.rs Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> * Update frame/staking/src/lib.rs Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> * Update frame/staking/src/lib.rs Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> * Update primitives/runtime/src/traits.rs Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> * Formatting Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Co-authored-by: kianenigma <kian@parity.io>
Polkadot companion: paritytech/polkadot#4500
Cumulus companion: paritytech/cumulus#842
Having a default
AccountId
effectively privileges the zero account as a special case, to be used when "no other account makes sense". This should obviously not be the case.AccountId
conceptually does not have a default value and therefore the implementation should not exist and pallets should not assume it does.There are several API ramifications of this change, though all of them have simple solutions.
Code migration
SignedExtension
Pre
no longer has aDefault
bound. This means that the default implementation forpre_dispatch
(which executes thevalidate
function and returns a defaultPre
value) cannot exist and must be defined at the implementation site. There is a "standard" implementation which is functionally equivalent to the previous default implementation as long asPre
is()
:This can generally just be dropped in to a
SignedExtension
impl which was not otherwise defining it.Authoring pallet
fn author
pallet_authoring::author()
now returns anOption
, which isNone
if the block author is unknown. This should not typically be the case on production chains, but often is the case for tests. A future question is whether we really want to support this or rather to force tests to introduce a dummy author. The easiest way to transition is wrap any usage in anif let Some(author) = author() { ... }
.Session keys in tests
Session pallet now forces the existence of session keys for validators whereas previously it used default keys if they had not been set. This means that tests must explicitly set any session keys of validator accounts if they expect to see the account in the list of elected validators.
Public
trait split intoByteArray
The functions within the
sp_core::crypto::Public
trait which worked on the assumption that the underlying type was a fixed-sizeu8
array are now in a new traitByteArray
.Public
implies this. Generally you shoulduse sp_core::crypto::ByteArray;
instead ofPublic
.from_slice
is fallibleThe function
ByteArray::from_slice
(which used to bePublic::from_slice
) has been changed to return aResult
in the case that the slice provided was the incorrect size. For an infallible conversion, use theUncheckedFrom
trait with a fixed size array.Sudo pallet
GenesisConfig
Sudo pallet's
GenesisConfig::key
parameter is now anOption<AccountId>
since a safe default value for the field must be provided. Wrap any previous value inSome
, so that what used to be:Now becomes: